recording-daemon: lifecycle notification events by goolanceman · Pull Request #2142 · sipwise/rtpengine

goolanceman · 2026-07-18T17:49:55Z

Summary

Extends rtpengine-recording notifications from the historical finished-only path to a configurable call/stream lifecycle event set, while remaining backward compatible.

Events (`notify-events` CSV)

Token	Event name
`opened`	`recording_file_opened`
`started`	`recording_started`
`finished` (default)	`recording_finished`
`discarded`	`recording_discarded`
`failed`	`recording_failed`
`call-started`	`call_recording_started`
`call-finished`	`call_recording_finished`
`call-discarded`	`call_recording_discarded`
`all`	all of the above

Default remains finished only so existing deployments are unchanged.

New options

notify-events — CSV list (default finished)
notify-json — POST JSON body via json-glib
notify-no-metadata — omit call metadata from payloads
notify-command-format — legacy | extended | json-env
notify-queue-limit — bound non-terminal queue depth (default 1000; 0 = unlimited). Terminal events are always accepted.

Design

Keeps upstream action-based notify model (notif_action_t / notif_req_t) and lib/http.c helpers
Pure event helpers in notify_events.c (shared with unit tests)
Stream hooks in output.c; call hooks in metafile.c / packet.c
Snapshots strings into notif_req before queueing (workers never touch live objects)
Metrics logged on shutdown: enqueued / success / retry / giveup / dropped

Tests

Unit: t/test-notify-events.c — PASS
E2E smoke: t/notify_e2e/ temp Python HTTP receiver + metafile lifecycle — E2E_NOTIFY_OK
- Built/ran in Docker
- Received call_recording_finished with JSON body + lifecycle headers

Docs

docs/rtpengine-recording.md
etc/rtpengine-recording.conf

Example

notify-uri = https://example.com/rec/events
notify-events = opened,started,finished,discarded,failed,call-started,call-finished,call-discarded
notify-json = true
notify-concurrency = 5
notify-retries = 10

Compatibility

Default config behavior is identical to previous finished-only notifications
Legacy notify-command argv (path, db_id) preserved when notify-command-format=legacy

I used an AI assistant to help with some of the repetitive work (e.g. scaffolding, draft text), but the architecture, behaviour, and all final code changes were reviewed, adapted, and validated by me. The goal is to speed up the mechanical parts while still having a human developer take responsibility for correctness, style, and alignment with rtpengine 's conventions. Feedback is very welcome and I'll iterate on the implementation as needed.

rfuchs

This is all AI generated, isn't it

goolanceman · 2026-07-20T05:50:11Z

This is all AI generated, isn't it

Good eye 🙂
I did use an AI assistant to help draft parts of this (especially the docs/text), but I've reviewed and adjusted everything myself and verified the code and tests. I'm fully responsible for the changes here, so if anything looks off, please call it out and I'll fix it.

Extend rtpengine-recording HTTP/command notifications beyond the historical finished-only path so operators can track call and stream recording lifecycle. Events (notify-events CSV, default: finished): opened, started, finished, discarded, failed, call-started, call-finished, call-discarded New options: notify-events, notify-json, notify-no-metadata, notify-command-format, notify-queue-limit. Docs/conf only document the new notify options; upstream storage (S3/GCS/output-storage) docs are left unchanged. Backward compatible: default mask is finished only. Includes unit tests and Docker e2e smoke harness.

notify_setup must not require notify-uri/notify-command: the same thread pool backs S3, GCS and DB notif_action_t work. Also leave object_name to action cleanup to avoid double-free on those paths.

rfuchs

Not sure if I'm willing to accept this as it all looks like an overly complicated wall of code, typical of AI slop

rfuchs · 2026-07-22T11:57:18Z

+static void notify_req_snapshot(notif_req_t *req, enum notify_event event,
+		output_t *o, metafile_t *mf, tag_t *tag,
+		const char *error_code, const char *error_message)
+{
+	req->event = event;
+	req->terminal = notify_event_is_terminal(event) ? 1 : 0;
+
+	if (mf) {
+		req->call_id = g_strdup(mf->call_id);
+		req->call_start = (double) mf->start_time_us / 1000000.;
+		req->call_db_id = mf->db_id;
+		if (!notify_no_metadata && mf->metadata)
+			req->metadata = g_strdup(mf->metadata);
+	}


This whole function seems redundant? It copes all the fields into a notif_req_t, only to have the fields copied again in notify_enqueue() and then freed?

rfuchs · 2026-07-22T12:03:46Z

+	const char *call_id = req->call_id ? req->call_id : (mf && mf->call_id ? mf->call_id : NULL);
+	const char *file_name = req->file_name ? req->file_name : (o && o->file_name ? o->file_name : NULL);
+	const char *file_format = req->file_format ? req->file_format : (o && o->file_format ? o->file_format : NULL);
+	const char *kind = req->kind ? req->kind : (o && o->kind ? o->kind : NULL);
+	const char *full_filename = req->full_filename ? req->full_filename :
+		(o && o->full_filename ? o->full_filename : NULL);
+	const char *output_id = req->output_id ? req->output_id : (o && o->output_id ? o->output_id : NULL);
+	const char *metadata = req->metadata ? req->metadata :
+		(!notify_no_metadata && mf && mf->metadata ? mf->metadata : NULL);
+	double call_start = req->call_start > 0 ? req->call_start :
+		(mf ? (double) mf->start_time_us / 1000000. : 0);
+	double stream_start = req->stream_start > 0 ? req->stream_start :
+		(o ? (double) o->start_time_us / 1000000. : call_start);
+	unsigned long long call_db_id = req->call_db_id ? req->call_db_id : (mf ? mf->db_id : 0);


The call ID and the other fields are copied from the metafile_t or the output anyway. What's the reason for having all these case distinctions here?

rfuchs · 2026-07-22T12:07:32Z

+				notif_req_t *first = rtpe_g_tree_first(notify_timers);
+				if (!first)
+					break;
+				g_tree_remove(notify_timers, first);
+				nont_inflight_dec(first);
+				if (first->action && first->action->cleanup)
+					first->action->cleanup(first);
+				req_free_fields(first);
+				g_free(first->name);
+				g_free(first);
+			}
+			g_tree_destroy(notify_timers);
+			notify_timers = NULL;


This seems like a candidate for g_tree_foreach ?

goolanceman marked this pull request as ready for review July 18, 2026 17:53

goolanceman force-pushed the recording-notify-lifecycle branch from a4ccba2 to fd097da Compare July 18, 2026 18:10

rfuchs reviewed Jul 19, 2026

View reviewed changes

Comment thread recording-daemon/notify.c

Comment thread recording-daemon/notify.c

goolanceman force-pushed the recording-notify-lifecycle branch from a6f6824 to bf74310 Compare July 20, 2026 06:14

Mansoor Khan M added 4 commits July 21, 2026 12:52

recording-daemon: keep notify thread pool for S3/GCS/DB

d30d1ba

notify_setup must not require notify-uri/notify-command: the same thread pool backs S3, GCS and DB notif_action_t work. Also leave object_name to action cleanup to avoid double-free on those paths.

notify: restore debug logs/comments and avoid reformat-only changes

8f9ea7d

style: tidy notify.c formatting and clarify ownership comment

a1ae952

goolanceman force-pushed the recording-notify-lifecycle branch from 70d8ba9 to a1ae952 Compare July 21, 2026 07:22

rfuchs reviewed Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

recording-daemon: lifecycle notification events#2142

recording-daemon: lifecycle notification events#2142
goolanceman wants to merge 4 commits into
sipwise:masterfrom
goolanceman:recording-notify-lifecycle

goolanceman commented Jul 18, 2026 •

edited

Loading

Uh oh!

rfuchs left a comment

Uh oh!

Uh oh!

Uh oh!

goolanceman commented Jul 20, 2026

Uh oh!

rfuchs left a comment

Uh oh!

rfuchs Jul 22, 2026

Uh oh!

rfuchs Jul 22, 2026

Uh oh!

rfuchs Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

goolanceman commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Events (notify-events CSV)

New options

Design

Tests

Docs

Example

Compatibility

Uh oh!

rfuchs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

goolanceman commented Jul 20, 2026

Uh oh!

rfuchs left a comment

Choose a reason for hiding this comment

Uh oh!

rfuchs Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

rfuchs Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

rfuchs Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

goolanceman commented Jul 18, 2026 •

edited

Loading

Events (`notify-events` CSV)